Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(unstable/test): imperative test steps API #12190

Merged
merged 27 commits into from
Oct 11, 2021

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Sep 22, 2021

Implementation of #10771 and part of #11901.

This is missing permissions and metadata, but those can be added in a future PR (likely easier once permissions stabilize).

Copy link
Contributor

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test coverage! but this changes a lot of unnecessary things (undoing things I've more or less just refactored to be that way, like the enums) and seems overly complex.

Also #11736 has been open for a month pending consensus, looks like they're at about the same level of completeness (mine can stream output tho).

@dsherret
Copy link
Member Author

dsherret commented Sep 23, 2021

but this changes a lot of unnecessary things (undoing things I've more or less just refactored to be that way, like the enums) and seems overly complex.

Could you point out the specific areas you mean in the PR by commenting on them? As for the complexity, I believe it is necessary to implement this feature based on the RFC as there are a lot of conditions that need to be enforced at runtime. I'm sure there are a few areas that could be refactored to be a little cleaner though as this is just a first pass.

Also #11736 has been open for a month pending consensus, looks like they're at about the same level of completeness (mine can stream output tho).

I did not see this PR. I'm sorry about that and feel bad... nobody told me about it. In the future though it would be helpful to open an issue for work that you're working/going to be working on to signal that you're doing that work. Then once done linking the PR to that issue. For example, I opened #11901 and assigned it to myself to signal I was going to work on this a few weeks ago... it would have been helpful to have commented on that issue with your PR to let me know that this had already been done. Though that said, I should have reached out to you about this one this week just to ensure we weren't duplicating work. Oh well... perhaps we could combine some aspects of the two PRs though.

I don't think the streaming output works properly with the approach in that PR with test steps executed in parallel. The output will trample all over itself. With this PR we could improve it to be streaming, but this could only be done reliably when a test has sanitizers and we know that it will be executing one at a time. It would then need to handle switching between the two possibilities. I think that could be a future improvement and perhaps we could use the API from your PR to help do that.

@dsherret dsherret marked this pull request as ready for review September 23, 2021 00:24
@@ -113,26 +113,47 @@ declare namespace Deno {
* See: https://no-color.org/ */
export const noColor: boolean;

export interface TestDefinition {
fn: () => void | Promise<void>;
export interface Tester {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a different name, Tester is the name of the actual main test runner type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a different suggestion? TestContext?

Copy link
Contributor

@caspervonb caspervonb Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestContext or just Test work

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to TestContext.

Copy link
Contributor

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking better but still, like I've been saying offline I would just go with the existing PR rather than putting in the work on making this landable.

If you're going to keep at this it would be useful to differentiate or do something different here. Maybe showcase how metadata works and how that provides us with the tight deferred test runner controlled execution can be polyfilled with that as was mentioned in #11901?

Premise put forth seems to be that we don't need tight integration with the test runner, I'd like to see that proven before allowing either steps implementations to be merged.

@dsherret dsherret force-pushed the imperative_test_steps branch from 437f4fc to af3e503 Compare September 29, 2021 01:19
@dsherret
Copy link
Member Author

Looking better but still, like I've been saying offline I would just go with the existing PR rather than putting in the work on making this landable.

The duplicate PR is unfortunate, but there is still some scenarios not handled by #11736 and it also has filtering which hasn't been discussed (we probably should add that in a follow up PR).

If you're going to keep at this it would be useful to differentiate or do something different here. Maybe showcase how metadata works and how that provides us with the tight deferred test runner controlled execution can be polyfilled with that as was mentioned in #11901?

Premise put forth seems to be that we don't need tight integration with the test runner, I'd like to see that proven before allowing either steps implementations to be merged.

Like you, I'm still not convinced we should go with metadata, but it would work. For now, I've moved this API to unstable.

@dsherret dsherret force-pushed the imperative_test_steps branch from 4afc489 to 2291680 Compare September 30, 2021 15:04
@lucacasonato
Copy link
Member

This is working great with my mocha polyfill:

import "https://gist.githubusercontent.com/lucacasonato/54c03bb267074aaa9b32415dbfb25522/raw/fc2cb1d656c733f76a5db6792fa3a26b9b683e33/deno_mocha.ts";

describe("group", () => {
  it("hello", () => {
    throw new Error("Hey!");
  });

  it("world", () => {
    1 + 2;
  });
});

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. The assertions to prevent footguns are all there. I'm happy with landing this in unstable.

@bartlomieju
Copy link
Member

This is working great with my mocha polyfill:

import "https://gist.githubusercontent.com/lucacasonato/54c03bb267074aaa9b32415dbfb25522/raw/fc2cb1d656c733f76a5db6792fa3a26b9b683e33/deno_mocha.ts";

describe("group", () => {
  it("hello", () => {
    throw new Error("Hey!");
  });

  it("world", () => {
    1 + 2;
  });
});

Just FYI it seems Mocha can already work with Deno: https://github.com/denoland/deno_std/pull/1334/files#diff-fa7e75c2cff0b94ed10f5be1e8c056c56d52ca0f8fcb6e3ec3a2c2aed37291af

@dsherret dsherret requested a review from lucacasonato October 5, 2021 21:29
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it!

@dsherret dsherret changed the title feat(test): imperative test steps API feat(unstable/test): imperative test steps API Oct 11, 2021
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -0,0 +1,4 @@
Deno.test("description", async (t) => {
// deno-lint-ignore no-explicit-any
await (t as any).step("step", () => {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is (t as any) required? I thought TS could infer that it's TestContext

Copy link
Member Author

@dsherret dsherret Oct 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no step method in the type declarations when not in unstable.

@dsherret dsherret merged commit 426ebf8 into denoland:main Oct 11, 2021
@dsherret dsherret deleted the imperative_test_steps branch October 11, 2021 13:45
@N8Brooks
Copy link

N8Brooks commented Oct 12, 2021

Is it supposed to count how many Deno.tests there are or how many t.stepss there are? Right now it seems to count how many Deno.tests there are even though there are multiple t.stepss. For example:

test day01 ...
  test part1 ...
    test example1 ... ok (8ms)
    test example2 ... ok (7ms)
    test example3 ... ok (7ms)
    test data ... ok (8ms)
  ok (40ms)
  test part2 ...
    test example1 ... ok (7ms)
    test example2 ... ok (8ms)
    test data ... ok (8ms)
  ok (31ms)
ok (79ms)

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out (105ms)

It says "1 passed" even though there are several t.stepss.

Very cool feature! This will help a lot organizing tests!

@bartlomieju
Copy link
Member

CC @dsherret

@dsherret
Copy link
Member Author

dsherret commented Oct 12, 2021

@N8Brooks technically there is only 1 test there and there are 2 steps with each their own steps underneath them. So the final count is only the tests that have passed and not how many steps there were to do that test. Perhaps #12411 would make this more clear?

@hayd
Copy link
Contributor

hayd commented Nov 3, 2021

TestContext broke tests in deno-lambda's serverless example, which was doing its own setup/teardown:

https://github.com/hayd/deno-lambda/blob/debc93d9416bc546a06e139276b57367ca45da45/example-serverless/test_util.ts#L9

Are we supposed to just stub in fn(undefined as any) ? 😳

Is there an issue/discussion for TestDefinition?

@dsherret
Copy link
Member Author

dsherret commented Nov 3, 2021

@hayd you may just want to provide:

fn({
  step() {
    throw new Error("Test steps are not supported by deno-lambda at this time.");
  }
} as any)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants